Skip to content

Fix RequestsCookieJar MutableMapping mismatch#7251

Open
EthicalCha0s wants to merge 1 commit intopsf:mainfrom
EthicalCha0s:fix/cookiejar-mutablemapping-mismatch
Open

Fix RequestsCookieJar MutableMapping mismatch#7251
EthicalCha0s wants to merge 1 commit intopsf:mainfrom
EthicalCha0s:fix/cookiejar-mutablemapping-mismatch

Conversation

@EthicalCha0s
Copy link

Summary

RequestsCookieJar currently inherits from both CookieJar and
MutableMapping, but it does not actually satisfy the MutableMapping
contract.

CookieJar.__iter__ yields Cookie objects, while MutableMapping expects
iteration over keys. That means RequestsCookieJar can report itself as a
MutableMapping even though code written against that interface will fail.

This change removes the MutableMapping base class from
RequestsCookieJar while keeping its existing dict-like convenience methods.

What changed

  • removed MutableMapping from RequestsCookieJar's base classes
  • kept the existing dict-like API (get, set, keys, items, etc.)
  • added explicit implementations for behavior that had been inherited from
    MutableMapping:
    • __contains__
    • update
    • __eq__
  • added a regression test to verify RequestsCookieJar is no longer treated
    as a MutableMapping

Why this approach

I considered changing iteration to yield keys instead, but that would also
change the current behavior of iterating over a cookie jar, which today yields
Cookie objects.

Removing the incorrect ABC inheritance seemed safer than changing iteration
semantics.

Testing

Ran a focused local test slice covering the changed behavior:

  • tests/test_requests.py -k 'prepared_copy or cookie_jar_is_not_a_mutable_mapping or cookie_as_dict or cookie_parameters or cookie_duplicate_names'

Result:

  • 13 passed

I also compared a full local test run on a clean clone versus this branch.

Clean clone:

  • 400 passed
  • 15 skipped
  • 1 xfailed
  • 196 errors

Patched branch:

  • 401 passed
  • 15 skipped
  • 1 xfailed
  • 196 errors

The extra passing test is the new regression test. The 196 errors were already
present on a clean clone in my environment.

Closes #7228

@EthicalCha0s
Copy link
Author

Quick follow-up: after installing the dev test dependencies from requirements-dev.txt, I reran the full test suite on this branch and got:

  • 597 passed
  • 15 skipped
  • 1 xpassed
  • 0 failed

So the earlier big batch of errors was down to missing test dependencies in my local environment, not this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RequestsCookieJar shouldn't inherit from MutableMapping

1 participant